Skip to content

Type hash distribution in discovery (rep2011)#437

Merged
clalancette merged 15 commits intoros2:rollingfrom
emersonknapp:emersonknapp/type-version-hash
Mar 26, 2023
Merged

Type hash distribution in discovery (rep2011)#437
clalancette merged 15 commits intoros2:rollingfrom
emersonknapp:emersonknapp/type-version-hash

Conversation

@emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Feb 20, 2023 •
edited
Loading

Part of ros2/ros2#1159

Depends on ros2/rosidl#729
Depends on ros2/rmw_dds_common#70

Features:

  • Retrieve type version hash from generated code, and use the USER_DATA QoS policy to distribute it for all readers/writers. Format of the USER_DATA uses common utility functions in rmw_dds_common
  • On discovering new endpoints, parse the USER_DATA to set rmw_topic_endpoint_info_t.type_hash

@emersonknapp emersonknapp mentioned this pull request Feb 20, 2023
Closed
14 tasks
@emersonknapp emersonknapp changed the title [WIP] Type hash distribution (REP-2011) [WIP] Type hash distribution in discovery (rep2011) Feb 21, 2023
eboasson
eboasson reviewed Feb 21, 2023
View reviewed changes
@emersonknapp emersonknapp changed the title [WIP] Type hash distribution in discovery (rep2011) Type hash distribution in discovery (rep2011) Feb 23, 2023
@emersonknapp emersonknapp marked this pull request as ready for review February 23, 2023 21:51
@emersonknapp emersonknapp requested review from eboasson and removed request for ivanpauno March 13, 2023 07:24
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 13, 2023

@eboasson @ivanpauno this is ready for review

clalancette
clalancette requested changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few minor things to fix.

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
}
if ((qos = create_readwrite_qos(qos_policies, false)) == nullptr) {

type_hash = get_type_hash(type_support);
Copy link
Contributor

@clalancette clalancette Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just do:

Suggested change
type_hash = get_type_hash(type_support);
rosidl_type_hash_t type_hash = get_type_hash(type_support);

Copy link
Contributor Author

@emersonknapp emersonknapp Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it first this way because it's a compiler error for the declaration to come after the first goto in the function. But I'm resolving by just moving the whole thing higher up, in both cases

@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch 4 times, most recently from 4fde0bc to 94e6153 Compare March 16, 2023 02:12
@emersonknapp emersonknapp requested review from clalancette and removed request for eboasson March 16, 2023 02:30
clalancette
clalancette requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes to do, otherwise this looks good.

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 17, 2023 •
edited
Loading

Same run combined for PRs ros2/rmw_dds_common#70 ros2/rmw_fastrtps#671 #437 ros2/rmw_connextdds#104

Gist: https://gist.githubusercontent.com/emersonknapp/597121a37b82d9418b14472de760c8f0/raw/b0412d81c2ae2368cb592c810496d6f92650ad68/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11623

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

This was referenced Mar 17, 2023
Merged
Merged
Merged
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 21, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

eboasson
eboasson approved these changes Mar 22, 2023
View reviewed changes
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

clalancette
clalancette requested changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the leak I think we have, this is also going to need a rebase onto the latest to pull in the latest MATCHED PRs that were merged a couple of days ago.

rmw_cyclonedds_cpp/src/rmw_node.cpp
rosidl_type_hash_t type_hash = rosidl_get_zero_initialized_type_hash();
void * userdata;
size_t userdata_size;
if (dds_qget_userdata(s->qos, &userdata, &userdata_size)) {
Copy link
Contributor

@clalancette clalancette Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we are leaking the userdata here. At least, the implementation in https://github.com/eclipse-cyclonedds/cyclonedds/blob/11b923aceb2faa506459fee6cfd708345569ac43/src/core/ddsc/src/dds_qos.c#L31 (which is what dds_qget_userdata calls) seems to allocate memory, and we don't seem to be freeing it anywhere.

Copy link
Contributor Author

@emersonknapp emersonknapp Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Rebased (all 4 prs) and added RCPPUTILS_SCOPE_EXIT(dds_free(userdata)); in the if block.

@emersonknapp
WIP type_version_hash just fix compile for API change
edc3465 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Starting to transfer type hash values, but need to interleave keyval …
7895b0c 
…with service ids in user_data

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
WIP user-data type hash passing
70cffca 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fetching request and response type hashes
ce2a99c 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
First pass type hash struct
216fcd9 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
use common encoder fn
ec2f14a 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Log type hash in discovery
1c645d6 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fix incorrect qos usage
46c8e21 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fix linter
773d192 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Use new fn signature
90ff03e 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Address review comments
d9581c0 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Minor reformatting
b14c4dd 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Add warning on type hash codec failure
f799d89 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Use new typesupport hashes
0d823ec 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from 661887b to 0d823ec Compare March 24, 2023 17:05
@emersonknapp
Free the user data
10938d6 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 24, 2023 •
edited
Loading

Gist: https://gist.githubusercontent.com/emersonknapp/c36ca5985030eccbcaac0d0384236fbd/raw/374f6ef848452efc7e6a2b079129195ece75aeec/ros2.repos
BUILD args: --packages-above-and-dependencies rmw_dds_common
TEST args: --packages-above rmw_dds_common
ROS Distro: rolling

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

clalancette commented Mar 26, 2023

CI is in ros2/rmw_dds_common#70 (comment)

@clalancette clalancette merged commit 1d7c8d1 into ros2:rolling Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@clalancette clalancette clalancette approved these changes

@eboasson eboasson eboasson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emersonknapp @clalancette @wjwwood @eboasson